Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for OpenSSL 3 #2

Closed
wants to merge 2 commits into from
Closed

Add support for OpenSSL 3 #2

wants to merge 2 commits into from

Conversation

xfalcox
Copy link
Contributor

@xfalcox xfalcox commented Dec 26, 2022

This is the same PR sent to the original and adjusted for this fork

See zaru/webpush#106

Co-authored-by: Claire claire.github-309c@sitedethib.com

@xfalcox @ClearlyClaire
Add support for OpenSSL 3
9b1ab76 
This is the same PR sent to the original and adjusted for this fork

See zaru/webpush#106

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
@collimarco
Copy link
Contributor

Thanks for working on this!!

  • I have reviewed the code, which seems ok to me, and the tests are passing. My main concern is about set_keys!: I wonder if there is a simpler alternative to create an OpenSSL::PKey::EC from existing base64 keys (also for performance)
  • Ruby team is still working on compatibility: Support OpenSSL 3.0 ruby/openssl#369
  • The Docker images for Ruby and Debian stable still use OpenSSL v1.1: OpenSSL 3 docker-library/ruby#397

@xfalcox
Copy link
Contributor Author

xfalcox commented Dec 27, 2022

The Docker images for Ruby and Debian stable still use OpenSSL v1.1: OpenSSL 3 docker-library/ruby#397

Our image too is based on debian stable, but without this booting the app in development is broken for every dev using Arch, Ubuntu, Fedora, or any up to date distro, which is too much friction for us.

No worries tho, we will fork this and use our fork then.

@collimarco
Copy link
Contributor

Yes, sure, you can use a fork in the meantime.

I will certainly merge this pull request (or equivalent) as soon as Debian stable uses OpenSSL v3 (i.e. when Bookworm becomes stable). Maybe I can merge even before that, but let me think about that.

Please feel free to suggest any other fixes / improvements to this library and I will be happy to merge them.

@collimarco
Copy link
Contributor

Related: ruby/openssl#369 (comment)

Basically the future versions of the openssl gem will provide a method for creating the curve directly from the keys, without ASN1. The new method is in C, so probably performance are better. In any case, at the moment, the correct solution is the one presented in this pull request.

@collimarco
Copy link
Contributor

@xfalcox I am planning to release a v3 of this gem with compatibility with OpenSSL v3.

I would like to merge this pull request, but it should not include the commit for Ruby 2.7, because it is not related to this issue.

  • Can you remove that second commit from this pull request so that I can merge? Can you also increase the major VERSION number of this gem to v3.0.0?
  • Otherwise, is it ok for you if I just copy your code and create the new release?

@collimarco
Copy link
Contributor

I have released v3 of this gem, which is compatible with both OpenSSL 1.1 and OpenSSL 3 🎉

I have also added @xfalcox and @ClearlyClaire to co-authors of the commit. Thanks for the contributions.

@collimarco collimarco closed this Jan 5, 2023
@xfalcox
Copy link
Contributor Author

xfalcox commented Jan 12, 2023

Cool, moved back into your fork in discourse/discourse#19849

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xfalcox @collimarco